Skip to content

SOLR-17316: make SolrJ response objects work with non-binary ResponseParsers - #4640

Open
serhiy-bzhezytskyy wants to merge 13 commits into
apache:mainfrom
serhiy-bzhezytskyy:SOLR-17316-proposal-response-normalizer
Open

SOLR-17316: make SolrJ response objects work with non-binary ResponseParsers#4640
serhiy-bzhezytskyy wants to merge 13 commits into
apache:mainfrom
serhiy-bzhezytskyy:SOLR-17316-proposal-response-normalizer

Conversation

@serhiy-bzhezytskyy

Copy link
Copy Markdown
Contributor

https://issues.apache.org/jira/browse/SOLR-17316

SolrJ's response classes assume binary-parser types, so reading a response parsed by a non-binary parser (the JSON parser) throws ClassCastException. Two commits so the small safe fix can go in on its own if you'd rather.

Commit 1 is the narrow fix: getStatus/getQTime cast to Integer, which CCEs when JSON gives Long. Widened via Number. That's it — resolves the getters the issue names.

Commit 2 goes further. It's not just the header — getResults(), facets, grouping all break under JSON too, because the parser hands back raw Map/List where the code wants NamedList/SolrDocumentList. So there's a ResponseNormalizer that converts a parsed response to the canonical shape at the client boundary (no-op for binary/XML), gated by a new ResponseParser.producesCanonicalForm() so only the JSON map parser triggers it. Both transports covered, binary pays nothing. Plus the remaining Integer/Float casts widened.

Heads up: commit 2 basically does what SOLR-3451 asked for in 2012, which was closed Won't Fix ("solr does not have a way to write a JSON response and read the same value"). Still true for json.nl=flat since it's lossy, but json.nl=map round-trips and the normalizer does the rest. So I'd treat commit 2 as reopening that discussion — fine to split it out or take it to dev@ if you'd prefer, commit 1 stands alone either way.

Tests: the normalizer + edge cases, binary/xml/json-map parity, the affected sections (grouping, facets, stats, spellcheck, highlighting, terms, moreLikeThis, analysis, Luke, schema), and an end-to-end JSON query on both the Jetty and JDK clients.

…arsers

getStatus() and getQTime() cast the header value to Integer, which threw a
ClassCastException under a parser that yields a different numeric type (the JSON
parser yields Long). Widen via Number.intValue() instead.
The SolrJ response classes assume the Java types the binary parser produces, so
reading a response parsed by a non-binary parser (e.g. the JSON map parser) threw
ClassCastException: JSON yields raw Map/List where the code expects
NamedList/SolrDocumentList, and Long where it casts to Integer.

- ResponseNormalizer converts a parsed response into the canonical shape (nested
  objects -> NamedList/SimpleOrderedMap, a {numFound,docs} object ->
  SolrDocumentList); it is a no-op for already-canonical binary/XML responses.
- ResponseParser.producesCanonicalForm() gates it; JsonMapResponseParser returns
  false. HttpSolrClient normalizes at the shared response boundary, covering both
  the JDK and Jetty transports while binary/XML pay nothing.
- Remaining numeric reads that cast to Integer/Float are widened via Number
  (grouping, interval and pivot facet counts, spellcheck, analysis token offsets,
  Luke, schema version).

Tests cover the normalizer, cross-format parity (binary/XML/json-map), each
affected section, and an end-to-end HTTP query with the JSON parser on both
transports.
@dsmiley
dsmiley requested a review from gerlowskija July 16, 2026 01:15
@dsmiley dsmiley added this to the 10.x milestone Jul 16, 2026
@serhiy-bzhezytskyy

Copy link
Copy Markdown
Contributor Author

Are there any objections to its merger? I am ready to resolve them, just let me know. Thanks

@dsmiley dsmiley left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work here!

Comment thread solr/solrj/src/java/org/apache/solr/common/util/ResponseNormalizer.java Outdated
Comment thread solr/solrj/src/java/org/apache/solr/common/util/ResponseNormalizer.java Outdated
Comment thread solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java Outdated
…e/style fixes

Review feedback on apache#4640.

The client no longer knows which parsers need normalizing. ResponseParser gains
processCanonicalResponse(), which defaults to processResponse() — most parsers are
canonical already and inherit it unchanged — and JsonMapResponseParser overrides it
to convert. The producesCanonicalForm() predicate is gone, and HttpSolrClient just
calls the one method:

  rsp = processor.processCanonicalResponse(is, encoding);

The existing processResponse() is untouched, since 20 call sites use it directly,
including the error path in ConcurrentUpdateBaseSolrClient which only reads
resp.get("error") and needs no conversion.

ResponseNormalizer moves from org.apache.solr.common.util to
org.apache.solr.client.solrj.response: it is not a common utility.

A plain NamedList is no longer promoted to SimpleOrderedMap. SimpleOrderedMap
implements Map and is written differently by the response writers -- a JSON writer
renders it as {"foo":10} and a NamedList as ["foo",10] -- so widening the type
changes the contract of the value. Only the concrete type is preserved now; a JSON
object still becomes a SimpleOrderedMap, since its keys are unique by construction.
Worth noting the mutation check: with "always promote" in place, all 27 existing
tests passed, so two tests were added for the distinction and they do fail on it.

Also: pattern-matching instanceof throughout ResponseNormalizer, text blocks for
the JSON literals in the parity test, no FQNs, and no try-with-resources around
solrTestRule.getSolrClient() -- its javadoc says "The caller doesn't need to close
it".

ResponseParserCanonicalFormTest becomes ResponseParserCanonicalResponseTest and
pins behaviour rather than the removed predicate: the JSON parser's raw output is
Maps, its canonical output is NamedLists and a SolrDocumentList, and a canonical
parser passes through unchanged.

200 solrj response/client tests pass; :solr:solrj:check clean.
… map parser

Extends the randomization SOLR-15070 introduced in that test — javabin or xml — to
pick among three parsers, so the JSON map parser goes through the same suggester
assertions as the other two.

It is a regression test for this PR rather than added coverage: on the commit
before the normalizer, forcing that parser in fails all three of the test's
methods with

  ClassCastException: class java.util.LinkedHashMap cannot be cast to class
  org.apache.solr.common.util.NamedList

and with the conversion in place they pass. Mutation-checked — removing the
conversion from JsonMapResponseParser#processCanonicalResponse brings the
ClassCastException back under -Ptests.iters=10.
The test fed the JSON parser a literal whose facet_fields was written in json.nl=map
form, which JsonMapResponseParser never requests, so the input shape does not occur on
a live request — running the response classes end-to-end over a real server surfaced
that same section failing as an array. Two of its three methods also asserted existing
javabin and xml behaviour rather than anything this change introduces.

Coverage of the numeric widening in QueryResponse is kept by
QueryResponseSectionParityTest, and of a real request by
QueryResponseJsonParserIntegrationTest.
…and have the JSON map parser ask for json.nl=map

JsonMapResponseParser could not read the response it was getting. Under the default
json.nl=flat a NamedList is written as an array of alternating names and values, so
facet_fields arrived as a List where the response classes expect a NamedList, and the
structure cannot be recovered after the fact. The style had to be set by hand on every
request, which is not something a caller should have to know per parser.

ResponseParser#getRequestParams supplies params alongside wt. Anything the request set
explicitly wins, so this only provides defaults. Applied in
HttpSolrClient#initializeSolrParams, which every HTTP client routes through.

QueryResponseJsonParserIntegrationTest no longer sets json.nl itself, which is what the
change is for, and asserts that an explicit value survives.
JSON has no document type, so the JSON writer emits nested documents as a
_childDocuments_ field holding a list of maps. The binary and XML parsers hand them back
as child documents; this one left them as a plain field, so SolrDocument#hasChildDocuments
was false and the children were unreachable through the documented accessors.

Nesting is recursive, so grandchildren are covered too.
facet_queries, a range facet's counts and a pivot's query counts were cast to
NamedList<Integer> and iterated as Integer entries, so a response whose numbers arrive as
Long threw ClassCastException. Same defect as the accessors already widened here, in three
places the earlier commits did not reach; the values are still narrowed to int, so
nothing about the public types changes.
SolrExampleTests has a subclass per parser and JSON was missing, so the whole 42-test
suite now runs against JsonMapResponseParser. That is what found the three defects fixed
in the preceding commits; on the commit before them it fails seven ways.

Nine assertions in SolrExampleTests pinned the boxed type of a number rather than its
value -- (Integer) getFieldValue(..), assertEquals(1.0f, ..), RangeFacet<Float, Float> --
and are relaxed to Number where the value is the point. Nothing is ignored: 42 of 42 pass,
and the binary, XML, CBOR and HTTP/2 subclasses are unaffected.
Comment thread solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java Outdated
Comment thread solr/solrj/src/java/org/apache/solr/client/solrj/response/ResponseParser.java Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you are supporting anonymous child documents -- which is the original thing and I've been meaning to deprecate it. Nowadays, we do "nested documents", which have named relationships from parent to child. Neither is reflected in the schema, but anyway you can fetch children (named or anonymous) via fl=*,[child fl=*] if I recall off the top of my head.

@dsmiley dsmiley Aug 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for this. It's good.

However, note that most of Lucene & Solr's randomized testing is done at a deeper level such that an individual test generally doesn't even have to do anything to get the randomization -- it just happens at a deeper test framework/infra level. For example... imagine if the default was a settable static supplier... and imagine if SolrTestCase were to set it. Then the useful test coverage would go through the roof (thousands of Solr tests) and we'd probably toss aside more of your tests as redundant. I'm hesitant to truly recommend precisely this... but I'm at least taking the educational opportunity of sharing the rather unique randomized testing philosophy that permeates the Lucene & Solr projects.

Comment thread changelog/unreleased/SOLR-17316-response-parsers.yml Outdated
Uses SolrParams.wrapDefaults and SolrParams.of instead of hand-rolled merging, and renames
getRequestParams to getAdditionalRequestParams. Parser-required params now take precedence
over the request's own, as wt already did — a parser that can't read the form the caller
asked for would fail rather than honour it.

EmbeddedSolrServer called processResponse directly, so a JSON parser there still threw
ClassCastException. It now applies the parser's params and reads the response canonically.
Named nested documents are reconstructed too, keyed on _nest_path_.
The only textual conflict was two imports in HttpSolrClient: SOLR-18351 removed
ContentStream from this file, SOLR-17316 added SolrParams. Compilation then
failed on ResponseNormalizerTest, because SOLR-18353 removed
SolrDocument.getChildDocumentCount() -- switched to getChildDocuments().size().

@dsmiley dsmiley left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work here @serhiy-bzhezytskyy ; I'm planning on merging Monday. It's ready now, honestly, just want to give @gerlowskija a little more time to look and to debate the changelog type.

SolrJ's QueryResponse and other response objects now work when the client is configured with a
non-binary response parser (such as the JSON parser); previously their accessors could throw a
ClassCastException.
type: fixed

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
type: fixed
type: added

Comment on lines +5 to +6
non-binary response parser (such as the JSON parser); previously their accessors could throw a
ClassCastException.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
non-binary response parser (such as the JSON parser); previously their accessors could throw a
ClassCastException.
JSON response parser (namely `JsonMapResponseParser`);
previously their accessors could throw ClassCastException.

I think we should just name JsonMapResponseParser specifically, as we know this works. JacksonDataBindResponseParser probably doesn't work. Albeit it'd be interesting to try experimentally. I'm definitely keen on use of Jackson for CBOR support. For Solr 11, I hope we can use Jackson inside JsonMapResponseParser instead of noggit, but that's a separate discussion.

Nothing changes for callers on the default binary response parser; the
benefit requires configuring a non-binary one, which is the opt-in
criterion dev-docs/changelog.adoc gives for "added". Retitled so the
entry states the new capability instead of opening on a
ClassCastException, which read wrong under an "Added" heading.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants